Fix AutoLogger log deletion and cleanup sorting bugs - #243

Please address the comments from this code review:

## Overall Comments
- In getFileTime(), epochCutoff is created as a UTC QDateTime while birthTime()/lastModified() are typically in LocalTime, so it would be safer to normalize the time specs (e.g., compare using birth.toUTC() and lastModified.toUTC() or construct epochCutoff with the same timeSpec) to avoid subtle timezone-based misclassifications.
- The autoLoggerLogicTest uses a MockFileInfo and getFileTimeMock rather than QFileInfo/getFileTime directly, which risks the test diverging from the real implementation over time; consider wiring the test to the actual getFileTime and QFileInfo (or a thin wrapper) so changes to the production helper are covered automatically.

## Individual Comments

### Comment 1
<location path="tests/TestGlobal.cpp" line_range="670-678" />
<code_context>
+
+void TestGlobal::autoLoggerLogicTest()
+{
+    // 1. Verify getFileTimeMock logic
+    MockFileInfo m1{ "f1.txt", QDateTime(QDate(1970, 1, 1), QTime(0, 0, 0), Qt::UTC), QDateTime(QDate(2023, 1, 1), QTime(0, 0, 0), Qt::UTC), 100 };
+    QCOMPARE(getFileTimeMock(m1), QDateTime(QDate(2023, 1, 1), QTime(0, 0, 0), Qt::UTC)); // Falls back to lastModified because birth is epoch 0 (<= 1980)
+
+    MockFileInfo m2{ "f2.txt", QDateTime(), QDateTime(QDate(2022, 1, 1), QTime(0, 0, 0), Qt::UTC), 100 };
+    QCOMPARE(getFileTimeMock(m2), QDateTime(QDate(2022, 1, 1), QTime(0, 0, 0), Qt::UTC)); // Falls back because birth is invalid
+
+    MockFileInfo m3{ "f3.txt", QDateTime(QDate(2024, 1, 1), QTime(0, 0, 0), Qt::UTC), QDateTime(QDate(2025, 1, 1), QTime(0, 0, 0), Qt::UTC), 100 };
+    QCOMPARE(getFileTimeMock(m3), QDateTime(QDate(2024, 1, 1), QTime(0, 0, 0), Qt::UTC)); // Uses birthTime because it is valid and > 1980
+
+    // 2. Verify sorting and cumulative size deletion logic
</code_context>
<issue_to_address>
**suggestion (testing):** Add a boundary test for birthTime exactly at the epoch cutoff (1980-01-01).

Since we only test invalid, epoch-0, and clearly post-1980 values, please add a case with `birthTime == 1980-01-01T00:00:00Z` to lock in the strict `>` behavior (i.e., that it still falls back to `lastModified`). This boundary is easy to regress if the comparison changes.
</issue_to_address>

### Comment 2
<location path="tests/TestGlobal.cpp" line_range="697-719" />
<code_context>
+    QCOMPARE(fileInfoList[1].name, QString("file2.txt")); // 2024
+    QCOMPARE(fileInfoList[2].name, QString("file1.txt")); // 2023
+
+    // Apply cumulative size check (limit is 100 bytes)
+    qint64 totalFileSize = 0;
+    QList<MockFileInfo> filesToDelete;
+    qint64 deleteWhenLogsReachBytes = 100;
+
+    for (const auto &fileInfo : fileInfoList) {
+        totalFileSize += fileInfo.size;
+        bool deleteFile = false;
+        if (totalFileSize >= deleteWhenLogsReachBytes) {
+            deleteFile = true;
+        }
+        if (deleteFile) {
+            filesToDelete.append(fileInfo);
+        }
+    }
+
+    // Expected:
+    // file3.txt kept (total 30, limit 100)
+    // file2.txt kept (total 80, limit 100)
+    // file1.txt deleted (total 140, limit 100)
+    QCOMPARE(filesToDelete.size(), 1);
+    QCOMPARE(filesToDelete[0].name, QString("file1.txt"));
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend size-based deletion tests to cover equality and multiple deletions scenarios.

To better cover `DeleteSize`, please also add a case where the cumulative size hits the limit exactly (verifying `>=` behavior) and another where it exceeds the limit enough that multiple files must be deleted. This will guard the threshold logic and ensure the loop deletes all required files.

```suggestion
    // Helper to apply cumulative size-based deletion with a given threshold
    auto collectFilesToDelete = [&](qint64 deleteWhenLogsReachBytes) {
        qint64 totalFileSize = 0;
        QList<MockFileInfo> filesToDelete;

        for (const auto &fileInfo : fileInfoList) {
            totalFileSize += fileInfo.size;
            if (totalFileSize >= deleteWhenLogsReachBytes) {
                filesToDelete.append(fileInfo);
            }
        }

        return filesToDelete;
    };

    // Scenario 1: limit is 100 bytes, only the oldest file should be deleted
    {
        const qint64 deleteWhenLogsReachBytes = 100;
        const QList<MockFileInfo> filesToDelete = collectFilesToDelete(deleteWhenLogsReachBytes);

        // Expected:
        // file3.txt kept (total 30, limit 100)
        // file2.txt kept (total 80, limit 100)
        // file1.txt deleted (total 140, limit 100)
        QCOMPARE(filesToDelete.size(), 1);
        QCOMPARE(filesToDelete[0].name, QString("file1.txt"));
    }

    // Scenario 2: cumulative size hits the limit exactly (verifies >= behavior)
    {
        const qint64 deleteWhenLogsReachBytes = 140; // total size of all three files
        const QList<MockFileInfo> filesToDelete = collectFilesToDelete(deleteWhenLogsReachBytes);

        // Expected:
        // file3.txt kept (total 30, limit 140)
        // file2.txt kept (total 80, limit 140)
        // file1.txt deleted when total reaches exactly 140
        QCOMPARE(filesToDelete.size(), 1);
        QCOMPARE(filesToDelete[0].name, QString("file1.txt"));
    }

    // Scenario 3: low limit so that multiple files must be deleted
    {
        const qint64 deleteWhenLogsReachBytes = 60;
        const QList<MockFileInfo> filesToDelete = collectFilesToDelete(deleteWhenLogsReachBytes);

        // Totals:
        // file3.txt: total 30  (< 60)  -> kept
        // file2.txt: total 80  (>= 60) -> deleted
        // file1.txt: total 140 (>= 60) -> deleted
        QCOMPARE(filesToDelete.size(), 2);
        QCOMPARE(filesToDelete[0].name, QString("file2.txt"));
        QCOMPARE(filesToDelete[1].name, QString("file1.txt"));
    }
}
```
</issue_to_address>

### Comment 3
<location path="tests/TestGlobal.cpp" line_range="668" />
<code_context>
+    return fileInfo.lastModified;
+}
+
+void TestGlobal::autoLoggerLogicTest()
+{
+    // 1. Verify getFileTimeMock logic
</code_context>
<issue_to_address>
**suggestion (testing):** Consider also covering the DeleteDays cleanup strategy with date-based mocks.

This test currently covers `getFileTime` and the `DeleteSize` regression. Since the PR description also calls out incorrect deletion under `DeleteDays` when `birthTime` is epoch 0 or invalid, please add a date-based scenario using `getFileTimeMock` (logs older/newer than `deleteWhenLogsReachDays`, including epoch-0 and invalid `birthTime`) to confirm that only truly old logs are removed.

Suggested implementation:

```cpp
    MockFileInfo m3{ "f3.txt", QDateTime(QDate(2024, 1, 1), QTime(0, 0, 0), Qt::UTC), QDateTime(QDate(2025, 1, 1), QTime(0, 0, 0), Qt::UTC), 100 };
    QCOMPARE(getFileTimeMock(m3), QDateTime(QDate(2024, 1, 1), QTime(0, 0, 0), Qt::UTC)); // Uses birthTime because it is valid and > 1980

    // 2. Verify DeleteDays cleanup logic using getFileTimeMock
    {
        // Simulated "now" and retention window
        const QDateTime now = QDateTime(QDate(2025, 1, 10), QTime(0, 0, 0), Qt::UTC);
        const int deleteWhenLogsReachDays = 3;
        const QDateTime cutoff = now.addDays(-deleteWhenLogsReachDays);

        // fOldBirthValid: valid birthTime, older than cutoff -> should be deleted
        MockFileInfo fOldBirthValid{
            "old_birth_valid.log",
            QDateTime(QDate(2025, 1, 1), QTime(0, 0, 0), Qt::UTC),   // 9 days old
            QDateTime(QDate(2025, 1, 2), QTime(0, 0, 0), Qt::UTC),
            100
        };

        // fOldEpochBirth: epoch birthTime (<= 1980) but old lastModified -> should be deleted via lastModified fallback
        MockFileInfo fOldEpochBirth{
            "old_epoch_birth.log",
            QDateTime(QDate(1970, 1, 1), QTime(0, 0, 0), Qt::UTC),   // epoch 0 (invalid for age)
            QDateTime(QDate(2025, 1, 1), QTime(0, 0, 0), Qt::UTC),   // 9 days old, older than cutoff
            100
        };

        // fRecentInvalidBirth: invalid birthTime, recent lastModified -> should NOT be deleted
        MockFileInfo fRecentInvalidBirth{
            "recent_invalid_birth.log",
            QDateTime(),                                            // invalid
            now.addDays(-1),                                        // 1 day old, newer than cutoff
            100
        };

        // fRecentEpochBirth: epoch birthTime, recent lastModified -> should NOT be deleted
        MockFileInfo fRecentEpochBirth{
            "recent_epoch_birth.log",
            QDateTime(QDate(1970, 1, 1), QTime(0, 0, 0), Qt::UTC),   // epoch 0
            now.addDays(-1),                                        // 1 day old, newer than cutoff
            100
        };

        // fRecentValidBirth: valid, recent birthTime -> should NOT be deleted
        MockFileInfo fRecentValidBirth{
            "recent_valid_birth.log",
            now.addDays(-1),                                        // 1 day old, newer than cutoff
            now.addDays(-1),
            100
        };

        const QList<MockFileInfo> files = {
            fOldBirthValid,
            fOldEpochBirth,
            fRecentInvalidBirth,
            fRecentEpochBirth,
            fRecentValidBirth
        };

        QStringList deletedFiles;
        for (const auto &file : files) {
            const QDateTime fileTime = getFileTimeMock(file);
            if (fileTime < cutoff) {
                deletedFiles << file.fileName;
            }
        }

        // Only files that are truly older than the cutoff (based on getFileTimeMock)
        // should be selected for deletion.
        const QStringList expectedDeletedFiles = {
            "old_birth_valid.log",
            "old_epoch_birth.log"
        };

        QCOMPARE(deletedFiles, expectedDeletedFiles);
    }

    // 3. Verify sorting and cumulative size deletion logic

```

If your DeleteDays cleanup logic is implemented in a helper (e.g. a function or method that already takes a list of files, `deleteWhenLogsReachDays`, and "now"), you can further improve this test by:
1. Replacing the manual loop that builds `deletedFiles` with a direct call to that helper, using the same `files`, `now`, and `deleteWhenLogsReachDays`.
2. Asserting on its output instead of the locally computed `deletedFiles`.

Adjust the container type (`QList`, `QVector`, etc.) and field name `file.fileName` if your actual `MockFileInfo` definition uses different identifiers.
</issue_to_address>

Please address the comments from this code review:

## Overall Comments
- The 1980 epoch cutoff logic is duplicated between `utils::getFileTime` and `getFileTimeMock`; consider exposing the cutoff as a shared constant or helper to keep tests aligned with production behavior as this logic evolves.
- In `deleteOldLogs`, `utils::getFileTime` is called multiple times per file (in the sort comparator and again in the DeleteDays check); consider caching the computed timestamp per `QFileInfo` to avoid redundant work and keep the cleanup logic simpler.

## Individual Comments

### Comment 1
<location path="tests/TestGlobal.cpp" line_range="715-724" />
<code_context>
+        const int deleteWhenLogsReachDays = 3;
</code_context>
<issue_to_address>
**suggestion (testing):** Add a DeleteDays boundary case where file age equals the threshold to verify the `>=` condition

Currently the test only exercises files clearly older (9 days) and newer (1 day) than the 3‑day threshold. Because production uses `>= conf.deleteWhenLogsReachDays`, please add a case where a file is exactly 3 days old and is deleted, and optionally a 2‑day‑old file that is retained, to validate the boundary behavior and guard against off‑by‑one errors.

Suggested implementation:

```cpp
    {
        // Simulated "now" and retention window
        const QDateTime now = QDateTime(QDate(2025, 1, 10), QTime(0, 0, 0), Qt::UTC);
        const int deleteWhenLogsReachDays = 3;
        const QDateTime cutoff = now.addDays(-deleteWhenLogsReachDays);

        // fOldBirthValid: valid birthTime, older than cutoff -> should be deleted
        MockFileInfo fOldBirthValid{"old_birth_valid.log",
                                    QDateTime(QDate(2025, 1, 1),
                                              QTime(0, 0, 0),
                                              Qt::UTC), // 9 days old
                                    QDateTime(QDate(2025, 1, 2), QTime(0, 0, 0), Qt::UTC),
                                    100};

        // Boundary case: file exactly at deleteWhenLogsReachDays (3 days old) should be deleted
        MockFileInfo fBoundaryBirthValid{"boundary_birth_valid.log",
                                         QDateTime(QDate(2025, 1, 7),
                                                   QTime(0, 0, 0),
                                                   Qt::UTC), // exactly 3 days old
                                         QDateTime(QDate(2025, 1, 7), QTime(0, 0, 0), Qt::UTC),
                                         100};

        // Boundary case: file just below the threshold (2 days old) should be retained
        MockFileInfo fBelowThresholdBirthValid{"below_threshold_birth_valid.log",
                                               QDateTime(QDate(2025, 1, 8),
                                                         QTime(0, 0, 0),
                                                         Qt::UTC), // 2 days old
                                               QDateTime(QDate(2025, 1, 8), QTime(0, 0, 0), Qt::UTC),
                                               100};

```

1. Ensure these new MockFileInfo instances are added to whatever collection or vector of files is passed to the deletion/cleanup logic in this test (e.g. append `fBoundaryBirthValid` and `fBelowThresholdBirthValid` alongside `fOldBirthValid` and any existing mocks).
2. Extend the test assertions to explicitly verify that `boundary_birth_valid.log` is deleted (because its age is exactly `deleteWhenLogsReachDays` and should match the `>=` condition) and that `below_threshold_birth_valid.log` is retained (because its age is less than the threshold).
3. If the test currently asserts on counts (e.g., number of remaining files) rather than specific filenames, update those expectations to reflect the additional boundary-case files.
</issue_to_address>